feat: Introducing Scheduler to Core Features (FM-814)#12
Open
bernhardccodev wants to merge 8 commits intomainfrom
Open
feat: Introducing Scheduler to Core Features (FM-814)#12bernhardccodev wants to merge 8 commits intomainfrom
bernhardccodev wants to merge 8 commits intomainfrom
Conversation
… made scheduler service a singleton (FM-814).
bernhardccodev
requested review from
ashwinnair-chimple,
janfb-codev and
miguelccodev
📝 WalkthroughWalkthroughAdds a Scheduler system: a Scheduler class for time-based callbacks, a TimeoutRegistry for component-scoped timers, a SchedulerService singleton exported from the public API, unit tests for scheduler/registry behavior, and README documentation describing usage and examples. Changes
Sequence DiagramsequenceDiagram
participant GameLoop as "Game Loop"
participant Service as "SchedulerService"
participant Scheduler as "Scheduler"
participant Registry as "TimeoutRegistry"
participant Component as "Component"
Component->>Service: createRegistry()
Service-->>Component: TimeoutRegistry
Component->>Registry: setTimeout(callback, delay)
Registry->>Scheduler: setTimeout(callback, delay)
Scheduler-->>Registry: TimerId
Registry-->>Component: TimerId
GameLoop->>Service: update(delta)
Service->>Scheduler: update(delta)
Scheduler->>Scheduler: advance timers, execute due callbacks
alt timer due
Scheduler->>Component: invoke callback
Scheduler->>Registry: remove fired TimerId
end
Service-->>GameLoop: update complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/index.ts (1)
8-9: Add missing scheduler type exports to root barrel.The scheduler module exports
TimerIdandTimeoutRegistrytypes, but the root barrel only re-exportsschedulerService. Since the package exposes only the root entrypoint (no public./schedulersubpath), consumers cannot access these types. Either export them fromsrc/index.tsor document that the scheduler feature is incomplete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 8 - 9, The root barrel currently re-exports only schedulerService but not the scheduler types, preventing consumers from using TimerId and TimeoutRegistry; update the root export file to also export those types by re-exporting TimerId and TimeoutRegistry from the scheduler module alongside schedulerService (i.e., add exports for the exported types TimerId and TimeoutRegistry from the module that provides schedulerService) so external consumers can import those types from the package root.src/scheduler/timeout-registry.ts (1)
1-1: Use type-only import forTimerId.
TimerIdis used exclusively in type positions (type annotations, generics). While your current TypeScript configuration doesn't enforce strict module syntax, usingtypeimports is a best practice for clarity and helps with tree-shaking.Suggested change
-import Scheduler, { TimerId } from './scheduler'; +import Scheduler, { type TimerId } from './scheduler';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scheduler/timeout-registry.ts` at line 1, The import currently brings TimerId as a value import even though it’s only used in type positions; update the import so TimerId is a type-only import (referencing the TimerId symbol) and keep Scheduler as the runtime import (referencing the Scheduler default). Change the import statement so TimerId is imported with the TypeScript "type" modifier (e.g., import Scheduler and import type { TimerId }) to avoid treating TimerId as a runtime value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/scheduler/scheduler.ts`:
- Around line 73-89: The update method currently subtracts delta and only fires
the callback once when remaining <= 0, causing intervals to "fall behind" after
large deltas; modify update (the update method that iterates this.timers and
uses timer.remaining, timer.delay, timer.loop, and this.cancel) so that when a
looping timer is overdue you repeatedly account for missed ticks: after reducing
timer.remaining by delta, use a loop (e.g., while (timer.remaining <= 0)) to
invoke timer.callback for each missed tick and add timer.delay to
timer.remaining each iteration until remaining > 0 (with a safety guard to avoid
infinite loops for non-positive delay values); for non-loop timers keep the
existing cancel behavior.
- Around line 1-3: The import and export of Opaque and TimerId are currently
treated as values; change the import to a type-only import (import type { Opaque
} from 'type-fest') and ensure TimerId is exported as a type (export type
TimerId = Opaque<number, 'TimerId'>) so both are treated as types only; update
any other references to TimerId (e.g., where TimerId is exported on line 103) to
use the type-only export form to avoid emitting value-level code.
In `@src/scheduler/timeout-registry.spec.ts`:
- Around line 47-55: The test currently only calls registry.cancel(id) which
doesn't verify removal from the internal store; update the assertion to inspect
the registry's internal timeouts collection directly after scheduler.update(500)
to ensure the fired timer was removed. Specifically, after calling
scheduler.update(500) and asserting the callback was called, assert that
TimeoutRegistry.timeouts (or registry.timeouts) does not contain the entry for
the id returned by registry.setTimeout(callback, 500); keep the existing
cancel-after-fire check but make the direct inspection the primary verification
that the timeout was auto-removed.
---
Nitpick comments:
In `@src/index.ts`:
- Around line 8-9: The root barrel currently re-exports only schedulerService
but not the scheduler types, preventing consumers from using TimerId and
TimeoutRegistry; update the root export file to also export those types by
re-exporting TimerId and TimeoutRegistry from the scheduler module alongside
schedulerService (i.e., add exports for the exported types TimerId and
TimeoutRegistry from the module that provides schedulerService) so external
consumers can import those types from the package root.
In `@src/scheduler/timeout-registry.ts`:
- Line 1: The import currently brings TimerId as a value import even though it’s
only used in type positions; update the import so TimerId is a type-only import
(referencing the TimerId symbol) and keep Scheduler as the runtime import
(referencing the Scheduler default). Change the import statement so TimerId is
imported with the TypeScript "type" modifier (e.g., import Scheduler and import
type { TimerId }) to avoid treating TimerId as a runtime value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b79c817f-7960-4620-adc6-55fe3a380b75
📒 Files selected for processing (6)
src/index.tssrc/scheduler/index.tssrc/scheduler/scheduler.spec.tssrc/scheduler/scheduler.tssrc/scheduler/timeout-registry.spec.tssrc/scheduler/timeout-registry.ts
bernhardccodev
changed the title
miguelccodev
approved these changes
Mar 10, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 40-41: The README imports use placeholder/incorrect packages;
replace both occurrences so they import from the public package that actually
exports these symbols: import schedulerService and the types TimeoutRegistry and
TimerId from '@curiouslearning/core' (either as a single combined import or two
imports), ensuring references to schedulerService, TimeoutRegistry, and TimerId
are imported from the same module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62bb4255-f3e2-43d7-a212-208f46f5d051
📒 Files selected for processing (2)
README.mdsrc/scheduler/timeout-registry.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/scheduler/timeout-registry.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
README.md (1)
40-41:⚠️ Potential issue | 🟠 MajorIncorrect package names remain from previous review.
These import statements still use placeholder/incorrect package names as flagged in the previous review. Both should import from
'@curiouslearning/core'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 40 - 41, Update the incorrect placeholder import sources so both lines import from '@curiouslearning/core': replace "import schedulerService from 'your-package';" with an import that pulls schedulerService from '@curiouslearning/core' and change "import type { TimeoutRegistry, TimerId } from '@curiouslearning/schedulerService';" to import the TimeoutRegistry and TimerId types from '@curiouslearning/core' instead; ensure the symbol names (schedulerService, TimeoutRegistry, TimerId) remain unchanged.
🧹 Nitpick comments (1)
README.md (1)
26-26: Consider simplifying "outside of" to "outside".The phrase "outside of" can be simplified to "outside" for more concise writing without loss of clarity.
✍️ Optional style improvement
-> **Note:** If used outside of `requestAnimationFrame` — for example in a custom loop that may produce high or unpredictable delta values — it is recommended to cap the delta time to prevent timers from misfiring: +> **Note:** If used outside `requestAnimationFrame` — for example in a custom loop that may produce high or unpredictable delta values — it is recommended to cap the delta time to prevent timers from misfiring:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 26, Update the phrasing in the README sentence that currently reads "If used outside of `requestAnimationFrame` — for example in a custom loop..." by replacing "outside of" with "outside" so it reads "If used outside `requestAnimationFrame` — for example in a custom loop..." to simplify wording while preserving meaning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 17: The import statement currently referencing
'@curiouslearning/schedulerService' is incorrect; update the import source to
'@curiouslearning/core' so that the schedulerService export is resolved (i.e.,
replace the module specifier in the import of schedulerService with
'@curiouslearning/core' wherever the symbol schedulerService is imported).
---
Duplicate comments:
In `@README.md`:
- Around line 40-41: Update the incorrect placeholder import sources so both
lines import from '@curiouslearning/core': replace "import schedulerService from
'your-package';" with an import that pulls schedulerService from
'@curiouslearning/core' and change "import type { TimeoutRegistry, TimerId }
from '@curiouslearning/schedulerService';" to import the TimeoutRegistry and
TimerId types from '@curiouslearning/core' instead; ensure the symbol names
(schedulerService, TimeoutRegistry, TimerId) remain unchanged.
---
Nitpick comments:
In `@README.md`:
- Line 26: Update the phrasing in the README sentence that currently reads "If
used outside of `requestAnimationFrame` — for example in a custom loop..." by
replacing "outside of" with "outside" so it reads "If used outside
`requestAnimationFrame` — for example in a custom loop..." to simplify wording
while preserving meaning.
ashwinnair-chimple
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
SchedulerandTimeout-Registryto core features.SchedulerServiceas a facade class that owns theSchedulersingleton internally.How to test
npm run test.Ref: FM-814
Summary by CodeRabbit